Skip to content

GUACAMOLE-1746: Docker Allow usage of custom keystore and custom certificat #805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

ronansalmon
Copy link
Contributor

The start script handles custom keystore/certificat.

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few spelling corrections, but I'm also concerned about the nature of the instructions added to the README.md file.

The instructions seem to cover a scenario where you're running guacd natively on a system but you're running guacamole-client in a Docker container. This is a perfectly valid scenario; however, I'm not sure that the README for guacamole-client within Docker should include instructions that are specific to guacd - either Docker-based or natively installed. It might be better to document that in either the gaucamole-server repo or the Docker chapter in the User Guide (guacamole-manual), and just put a note, here, that refers people to one of those two places?

@ronansalmon ronansalmon requested a review from necouchman April 5, 2023 15:33
Comment on lines +235 to +237
You need to create the new certificate on the guacd host, see https://github.com/apache/guacamole-server/blob/master/README
or https://github.com/apache/guacamole-server/blob/master/src/guacd-docker/README.md depending
on the version you will use (standalone vs docker).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we should document this somewhere in the Guacamole User Guide (Manual), and link to that instead of linking directly to the Github repository.

@necouchman
Copy link
Contributor

@ronansalmon Any chance we can wrap this one up?

@ronansalmon
Copy link
Contributor Author

Hey !
This PR is linked to apache/guacamole-server#419.
I was waiting for the PR 419 to be merged (if accepted) to go any further.

So, how should we proceed ?

@sirux88
Copy link
Contributor

sirux88 commented Oct 26, 2023

I would add some thoughts:
1) What is the environment variabale GUACD_SSL for? As far as I can see it's not used on guacamole-client nor on guacamole-server anywhere (except within the changed files). Seems obsolete to me
2) Maybe the environment variables GUACD_SSL_KEYSTORE_FILE and GUACD_SSL_KEYSTORE_PASS should be renamed to something fitting better to their true purpose like JAVA_KEYSTORE_FILE and JAVA_KEYSTORE_PASS

For reference: I had a similar approach here sirux88@e02d2be. My use case was using a self signed cert for LDAP

@mike-jumper
Copy link
Contributor

mike-jumper commented Oct 26, 2023

The GUACD_SSL environment variable controls the guacd-ssl property, which is used to enable SSL/TLS encryption between the webapp and guacd. It's uncommon to use this, as that communication is typically purely over loopback or at least on a trusted network route, but it's not obsolete.

@sirux88
Copy link
Contributor

sirux88 commented Oct 27, 2023

The GUACD_SSL environment variable controls the guacd-ssl property, which is used to enable SSL/TLS encryption between the webapp and guacd. It's uncommon to use this, as that communication is typically purely over loopback or at least on a trusted network route, but it's not obsolete.

I still can't find it. But if you're saying it's used I'm ok with this

@mike-jumper
Copy link
Contributor

mike-jumper commented Oct 27, 2023

The GUACD_SSL environment variable controls the guacd-ssl property, which is used to enable SSL/TLS encryption between the webapp and guacd. It's uncommon to use this, as that communication is typically purely over loopback or at least on a trusted network route, but it's not obsolete.

I still can't find it. But if you're saying it's used I'm ok with this

For reference, here's the code path:

Property Declaration (standardized as part of guacamole-ext)

/**
* Whether guacd requires SSL/TLS on connections.
*/
public static final BooleanGuacamoleProperty GUACD_SSL = new BooleanGuacamoleProperty() {
@Override
public String getName() { return "guacd-ssl"; }
};

Property Retrieval (via a GuacamoleProxyConfiguration from LocalEnvironment)

@Override
public GuacamoleProxyConfiguration getDefaultGuacamoleProxyConfiguration()
throws GuacamoleException {
// Parse guacd hostname/port/ssl properties
return new GuacamoleProxyConfiguration(
getProperty(Environment.GUACD_HOSTNAME, DEFAULT_GUACD_HOSTNAME),
getProperty(Environment.GUACD_PORT, DEFAULT_GUACD_PORT),
getProperty(Environment.GUACD_SSL, DEFAULT_GUACD_SSL)
);
}

(Voluntary) Usage of Retrieved Values

Implementations aren't strictly required to use getDefaultGuacamoleProxyConfiguration(), but most do, either explicitly or implicitly via SimpleConnection.

GuacamoleProxyConfiguration proxyConfig = environment.getDefaultGuacamoleProxyConfiguration();

GuacamoleProxyConfiguration defaultConfig = environment.getDefaultGuacamoleProxyConfiguration();

etc.

…YSTORE_PASS to JAVA_KEYSTORE_FILE and JAVA_KEYSTORE_PASS
@ronansalmon
Copy link
Contributor Author

ronansalmon commented Oct 30, 2023

I would add some thoughts: 1) What is the environment variabale GUACD_SSL for? As far as I can see it's not used on guacamole-client nor on guacamole-server anywhere (except within the changed files). Seems obsolete to me 2) Maybe the environment variables GUACD_SSL_KEYSTORE_FILE and GUACD_SSL_KEYSTORE_PASS should be renamed to something fitting better to their true purpose like JAVA_KEYSTORE_FILE and JAVA_KEYSTORE_PASS

@sirux88 Done

@tiborauer
Copy link

Hi, considering that this PR seems to be abandoned, I have also attempted to implement this in a generic way. Please see my repo and let me know whether it can be useful. My use case is similar to that of @sirux88, and this implementation provided a solution.

@ronansalmon
Copy link
Contributor Author

Hi, @tiborauer I gave up on this PR and #419. Feel free to close or take over on both of them.

@tiborauer
Copy link

I have created a new PR referring to the issues and the PRs. Since I have not created this one or own the repo, I'd rather leave it to you to close it.

@ronansalmon ronansalmon closed this Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants